Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Re-writing accessors transformer using the new AGP API #7694

Merged
merged 31 commits into from
Sep 16, 2022

Conversation

nhachicha
Copy link
Collaborator

@nhachicha nhachicha commented Jun 30, 2022

  • Migrating the Realm accessor Transformer to use the new AHP transformer API
  • Migrated Gradle plugin from Groovy to Kotlin.
  • Re-writing the internal object server transformer

Closes #7714

@cla-bot cla-bot bot added the cla: yes label Jun 30, 2022
@nhachicha nhachicha requested review from rorbech and cmelchior June 30, 2022 16:37
@nhachicha nhachicha linked an issue Jun 30, 2022 that may be closed by this pull request
ANDROID_BUILD_TOOLS=30.0.3
KOTLIN=1.5.31
KOTLIN_COROUTINES=1.5.2
KOTLIN=1.6.20

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Probably you can try 1.7 as is the latest stable available

@@ -1,5 +1,5 @@
distributionBase=GRADLE_USER_HOME
distributionPath=wrapper/dists
distributionUrl=https\://services.gradle.org/distributions/gradle-7.3.3-bin.zip
distributionUrl=https\://services.gradle.org/distributions/gradle-7.4-bin.zip

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

7.5 is also available

@nhachicha
Copy link
Collaborator Author

@rorbech I updated the current WIP

  • Currently, the JNI generated header files (.h) are not created if we invoke assemble, this might be caused by the various bumps to Gradle and AGP ... if you run compileBaseReleaseJavaWithJavac for instance then it will compile and generate the headers.
  • The internal transformer (the one that strips the ObjectServer symbols) needs to be re-written similarly to the accessor one.
  • I updated some tests projects to run the new transformer against them
  • We need to create a feature branch and release from it (probably using this template [DO NOT MERGE] manual release to maven central #7704

For reference:

@rorbech rorbech changed the base branch from master to feature/new-agp-transformer-api August 30, 2022 12:22
@rorbech rorbech marked this pull request as ready for review August 31, 2022 08:49
@rorbech
Copy link
Contributor

rorbech commented Aug 31, 2022

I manually verified that all our examples are running with the current state. Also verified that updating model and other sources is properly rebuilt in incremental builds for a couple of the examples.

Copy link
Contributor

@cmelchior cmelchior left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Awesome work 🥇 🕺 ... As I don't really know the new transform API it was a bit hard to track some of the changes, but if all our sample projects are building and tests are running, I think we can assume that things are working 🥳

I only left some minor comments.

gradle-plugin/src/main/templates/Version.kt Outdated Show resolved Hide resolved
gradle-plugin/src/main/templates/Version.kt Outdated Show resolved Hide resolved
val metadataCollector =
io.realm.buildtransformer.asm.visitors.AnnotationVisitor(annotationDescriptor)

forEachJarEntry { jarEntry, inputStream ->
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is a bit unclear why we iterate the jar entries twice, some documentation for each block would be nice.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added some comments

return@iterateDirs // Directory was deleted
forEachJarEntry { jarEntry, inputStream ->
val bytes = inputStream.use { inputStream ->
if (jarEntry.name.endsWith(".class")) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What happens with ressource files in the JAR? We seem to skip them here, but I'm not sure how they then end up in the outputprovider?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

They are handled by the else branch 🎉 🤪

private val inputs: ListProperty<Directory>,
private val allJars: ListProperty<RegularFile>,
private val referencedInputs: ConfigurableFileCollection,
private val output: RegularFileProperty) {
private lateinit var metadata: ProjectMetaData
private var analytics: RealmAnalytics? = null

init {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same, running all of this logic directly in the constructor feels ...iffy

Copy link
Contributor

@rorbech rorbech Sep 15, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same, I just exposed transform as internal and call it on the constructed object instead of implicitly from the init-block.

classPool.close();
fun copyProcessedClasses() {
for ((fqname: String, clazz: CtClass) in processedClasses) {
outputProvider.putNextEntry(JarEntry("${fqname.replace('.', '/')}.class"))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does this also work on Windows or should we use File.separatorChar?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would argue heavily that the Zip file entry header should be platform agnostic ... otherwise you would need to supply different Zip-files for each platform. And since it works on one platform then I assume that it would also work on the others.

@rorbech rorbech requested a review from cmelchior September 15, 2022 13:46
Copy link
Contributor

@cmelchior cmelchior left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💯

@rorbech rorbech merged commit a53c4b6 into feature/new-agp-transformer-api Sep 16, 2022
@rorbech rorbech deleted the nh/new_transformer branch September 16, 2022 09:36
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Mar 14, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Migrate to new transformer API RealmTransformer not executing on with AGP 7.2.0/1
7 participants